Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor API into new api package #3543

Merged
merged 21 commits into from
Oct 27, 2020
Merged

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Oct 15, 2020

Fixes #3316.

Changelog Entry

Added

  • Resolves #3316. Refactored platform-neutral APIs into the new api package, to be reused on React Native component, in PR #3543 by @compulim
    • The new layering is core -> api -> component (HTML-only) -> bundle
    • Includes composition mode, platform-neutral React hooks, and localization resources
    • Most hooks are available in the new api package. Some hooks are only available on the existing component package, due to their platform dependency or coupling with visual components. For example, Web Worker, 2D canvas, useMicrophoneButton* are not available on the api package
    • Most implementation of middleware are only available on component package due to their coupling with visual components or platform features. Some implementations, such as, card action middleware and activity grouping middleware, are available on api package. For example:
      • Carousel layout and stacked layout is only available on component package due to their coupling with their respective visual components
      • For card action middleware, imBack, messageBack and postBack actions are available on api package, but call, openUrl and other platform-dependent actions are only available on component package
    • activityMiddleware, attachmentMiddleware, etc, now support array for multiple middleware

Description

This is for refactoring platform-neutral APIs into a new api package, to be reused on React Native component.

Design

Please see #3316 for design.

In general, platform-neutral code are being moved to api package. Code that left on the component package assumes it is running inside a web browser.

We also improved error handling, because <ErrorBox> is a visual component and is platform-dependent. We need to update the error boundary component and render <ErrorBox> differently on different platforms.

Although both React and React Native should support styleSet, the content could be fundamentally different. Thus, styleSet are not offered in api packages, but in component and the future native-component package.

Specific Changes

  • Added new api package, layered between core and component
    • Moved most React hooks to the new api package
    • Moved localization resources to the new api package
    • Moved composition mode to the new api package
  • Split part of card action middleware into api package, while keeping some in the component package
    • imBack, messageBack, and postBack, which are platform-neutral, are moved to api package
    • call, openUrl, and others, are left untouched in the component package
  • Moved grouping activity middleware into api package
  • Improved error handling
    • Added new UserlandBoundary to add error boundary to user components (e.g. components rendered via middleware)
    • Added internalErrorBoxClass prop for internal use
      • Users of api package can pass their own React component (e.g. component package running in development mode)
      • api will render <ErrorBox> when error caught in error boundary, which in turn, will use the internalErrorBoxClass to render
    • When running in non-development mode (node_env !== 'development), component will pass undefined to internalErrorBoxClass and will hide the <ErrorBox>
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@corinagum
Copy link
Contributor

What's the capability: renderer change?

docs/HOOKS.md Outdated Show resolved Hide resolved
@corinagum corinagum self-assigned this Oct 19, 2020
Copy link
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions/comments.
Since this is P1, we should wait to merge until all P0s are in.

@corinagum corinagum assigned compulim and unassigned corinagum Oct 19, 2020
@@ -18,7 +18,11 @@ const ErrorBox = ({ error, type }) => {
<ScreenReaderText text={localize('ACTIVITY_ERROR_BOX_TITLE')} />
<div className={errorBoxStyleSet}>
<div>{type}</div>
<pre>{error.stack}</pre>
{/* The callstack between production and development are different, thus, we should hide it for visual regression test */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{/* The callstack between production and development are different, thus, we should hide it for visual regression test */}
{/* The callstack between production and development are different; therefore we should hide it for visual regression tests */}

@compulim compulim merged commit 0cd6b8d into microsoft:master Oct 27, 2020
@compulim compulim deleted the feat-api-package branch October 27, 2020 21:50
@compulim compulim mentioned this pull request Oct 29, 2020
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React Native: Split bf-wc-component into bf-wc-dom
2 participants